Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix url encoder by specifying allowed characters #51

Merged
merged 5 commits into from
Aug 16, 2017
Merged

Conversation

onmyway133
Copy link
Contributor

@onmyway133 onmyway133 commented Aug 16, 2017

Copy link

@JohnSundell JohnSundell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix 👍 Some comments inline, let me know what you think 🙂

@@ -1,4 +1,8 @@
import Foundation
#if os(OSX)
import Cocoa

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come this is needed, since you have only removed code from this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I added it by mistake. Will remove it

@@ -0,0 +1,25 @@
import Foundation

struct Utilities {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest using a different name than Utilities (which is both very ambiguous, and tends to be a "catch all" for all kinds of unrelated APIs). Since these utilities are all about encoding strings, how about implementing them in an extension on String?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ironically 😛 I just moved those from string extensions to a new class. I had some problem before when some frameworks have the same method on String

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's a valid point. In that case, how about a StringEncoder class or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I can add that


/// Perform url encoding
///
/// - Parameter string: The source string

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document parameter allowed

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I'd name that parameter allowedCharacters, since just allowed is a bit ambiguous in this context.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also why not just pass a CharacterSet, just like to String?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I will add that

@onmyway133 onmyway133 merged commit 110d789 into master Aug 16, 2017
@onmyway133 onmyway133 deleted the fix/encode branch August 16, 2017 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants